Merged
Conversation
Contributor
🧪 BenchmarkShould we run the Virtual MCP strategy benchmark for this PR? React with 👍 to run the benchmark.
Benchmark will run on the next push after you react. |
Contributor
Release OptionsShould a new version be published when this PR is merged? React with an emoji to vote on the release type:
Current version: Deployment
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 33 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/api/routes/decopilot/nats-stream-buffer.ts">
<violation number="1" location="apps/mesh/src/api/routes/decopilot/nats-stream-buffer.ts:119">
P2: The abort event listener added on `abortSignal` is never removed after the stream completes normally via `flush()`. This keeps the `publishDone` closure (and its captured `js`, `encoder`, `subj`) alive until the signal is GC'd. Consider removing the listener in `flush()`, or use `{ once: true }` combined with manual removal.</violation>
</file>
<file name="apps/mesh/src/web/components/chat/input.tsx">
<violation number="1" location="apps/mesh/src/web/components/chat/input.tsx:304">
P2: Avoid calling setTiptapDocLocal during render when the thread changes; move the reset into a useEffect tied to activeThreadId to prevent render-phase state updates.</violation>
</file>
<file name="apps/mesh/src/api/routes/decopilot/nats-cancel-broadcast.ts">
<violation number="1" location="apps/mesh/src/api/routes/decopilot/nats-cancel-broadcast.ts:30">
P2: start() returns before assigning onCancel when getConnection() is null, so local cancellations won’t fire if NATS isn’t available. Set onCancel before the early return so local cancel still works even without NATS.</violation>
</file>
<file name="apps/mesh/src/api/routes/decopilot/routes.ts">
<violation number="1" location="apps/mesh/src/api/routes/decopilot/routes.ts:544">
P2: `requestMessage` is re-saved (with new `created_at`/`updated_at` timestamps) in `onFinish` and `onStepFinish`, even though it was already persisted before streaming started. This overwrites the original creation timestamp and performs redundant writes. Consider only saving `responseMessage` in the subsequent calls, or skipping `requestMessage` if it was already persisted.</violation>
<violation number="2" location="apps/mesh/src/api/routes/decopilot/routes.ts:569">
P2: The `else if (memory)` branch in the catch block is unreachable dead code. `failThread` is always assigned immediately after `memory = mem` (with only a non-throwing function definition between them), so whenever `memory` is truthy, `failThread` is also truthy and the `if (failThread)` branch executes instead. This duplicates failure-handling logic and creates a risk of the two copies diverging.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/api/routes/decopilot/routes.ts">
<violation number="1" location="apps/mesh/src/api/routes/decopilot/routes.ts:500">
P2: The finish handler no longer retries saving the request message. Because saveMessagesToThread swallows errors, a transient failure before streaming can leave the thread missing the user message while the response is saved. Consider re-including requestMessage here to retry persistence.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
pedrofrxncx
force-pushed
the
background
branch
from
71cc199 to
c8dbfa5
Compare
Upgrade @ai-sdk/provider, @ai-sdk/react, and ai packages to latest minor versions for bug fixes and streaming improvements. Made-with: Cursor
Define typed CloudEvent schemas for decopilot real-time events: - step (tool calls, text deltas, reasoning) - finish (run completed/failed/cancelled) - thread-status (running/idle/failed) These types are consumed by the frontend SSE hooks and produced by the decopilot stream routes. Made-with: Cursor
Move NATS connection lifecycle into a dedicated NatsConnectionProvider so that event-bus, cancel-broadcast, and stream-buffer can share a single connection instead of each managing their own. Event bus factories now accept an optional NatsConnectionProvider rather than creating connections internally. Made-with: Cursor
… and run tracking Introduce three strategy interfaces for horizontal scaling: - CancelBroadcast: propagate run cancellation across pods (LocalCancelBroadcast for single-process, NatsCancelBroadcast for multi-pod) - StreamBuffer: replay AI stream chunks for late-joining clients (NoOpStreamBuffer for single-process, NatsStreamBuffer via JetStream) - RunRegistry: track active runs per-pod with abort controller lifecycle Each abstraction has a local fallback that works without NATS, making the system progressively enhanceable. Made-with: Cursor
… support Major restructure of the decopilot routes module: - Convert default export to createDecopilotRoutes() factory that accepts CancelBroadcast, StreamBuffer, and RunRegistry via DI - Runs now continue in the background after SSE disconnect, enabling tool calls to complete even when the client navigates away - Add cancel endpoint (POST /:org/decopilot/:threadId/cancel) that broadcasts cancellation across pods - Add attach endpoint (GET /:org/decopilot/:threadId/attach) for reconnecting to an in-progress run's stream - Emit typed SSE events (step/finish/thread-status) via sseHub - Optimize denyPendingApprovals to only scan the last assistant message - Extract validateThreadOwnership helper for thread-scoped endpoints - Wire NATS provider, strategies, and HMR cleanup in app.ts Made-with: Cursor
Split chat frontend into focused modules: - createSseSubscription: generic SSE → callback wiring (reused by workflow SSE and decopilot events) - useDecopilotEvents: subscribes to step/finish/thread-status SSE events and updates chat state in real-time - ChatState: lightweight zustand-like store for transient UI state (isRunning, current thread status) separate from server cache Chat context no longer manages streaming lifecycle directly — it delegates to the background run + SSE event model. The input component reacts to isRunning to show cancel affordance. Made-with: Cursor
pedrofrxncx
force-pushed
the
background
branch
from
c8dbfa5 to
7488f5c
Compare
Collaborator
Author
Contributor
|
@pedrofrxncx I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
6 issues found across 40 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/web/hooks/create-sse-subscription.ts">
<violation number="1" location="apps/mesh/src/web/hooks/create-sse-subscription.ts:65">
P2: Make the unsubscribe function idempotent; otherwise multiple calls can decrement refCount too far and close the shared EventSource while other subscribers are still active.</violation>
</file>
<file name="apps/mesh/src/web/components/chat/context.tsx">
<violation number="1" location="apps/mesh/src/web/components/chat/context.tsx:742">
P2: `resumeFailCountRef` is never reset when the active thread changes. If 3 resume failures accumulate on one thread, switching to a different in-progress thread will be unable to resume its stream. Reset the counter when the active thread changes.</violation>
</file>
<file name="apps/mesh/src/event-bus/nats-sse-broadcast.ts">
<violation number="1" location="apps/mesh/src/event-bus/nats-sse-broadcast.ts:40">
P2: start() now returns before assigning localEmit when no NATS connection is available, so local SSE broadcasts are silently dropped in the NATS-unavailable path. Set localEmit before the NATS availability check so local delivery still works even without a connection.</violation>
</file>
<file name="apps/mesh/src/web/components/chat/input.tsx">
<violation number="1" location="apps/mesh/src/web/components/chat/input.tsx:304">
P2: Avoid setting state during render when the thread changes. Move the reset into a useEffect keyed on activeThreadId to prevent render-time state updates and potential render loops.</violation>
</file>
<file name="apps/mesh/src/api/routes/decopilot/routes.ts">
<violation number="1" location="apps/mesh/src/api/routes/decopilot/routes.ts:530">
P1: Bug: `onStepFinish` is not a valid parameter of `createUIMessageStream` — it will be silently ignored. According to the AI SDK docs, `onStepFinish` is only supported on `streamText`/`generateText`. This means step counting, SSE step events, and periodic intermediate saves will never execute.
Move `onStepFinish` to the `streamText()` call (which already supports it) to restore step tracking.</violation>
</file>
<file name="apps/mesh/src/api/routes/decopilot/nats-stream-buffer.ts">
<violation number="1" location="apps/mesh/src/api/routes/decopilot/nats-stream-buffer.ts:179">
P2: Timer leak: each `pull()` iteration creates a `setTimeout` (30 s) via `Promise.race` that is never cleared when `iter.next()` wins the race. For a replay of many messages, this accumulates thousands of lingering timers. Store the timer ID and `clearTimeout` it after the race resolves.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Added a timer to ensure proper cleanup in NatsStreamBuffer during pull operations. - Enhanced abort signal handling in createDecopilotRoutes to prevent unnecessary updates. - Updated ChatProvider to persist partial messages in the thread cache, preventing UI inconsistencies during message source switches. - Refactored SSE subscription cleanup logic to prevent multiple unsubscriptions. These changes enhance the reliability and performance of the chat and streaming functionalities.
- Refactored denyPendingApprovals to handle multiple assistant messages and return the same reference when no changes are needed. - Added unit tests to verify the behavior of denyPendingApprovals across various scenarios, ensuring correct state updates for tool calls. - Updated Chat context to improve stability and reduce unnecessary re-renders by separating stable and streaming values. These changes improve the reliability of message handling in the chat system.
Contributor
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/web/components/chat/context.tsx">
<violation number="1" location="apps/mesh/src/web/components/chat/context.tsx:816">
P2: Avoid mutating refs during render; move the thread-switch reset into an effect so concurrent renders can’t leave the resume guards in an inconsistent state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Replaced the import of useChatStable from the chat component to the context module, enhancing the organization of chat-related hooks. - This change aims to streamline the chat functionality and improve overall stability in the task management interface.
- Updated ChatProvider to always persist streamed messages into the thread cache, preventing UI inconsistencies when switching message sources. - Refined notification logic to trigger only when the finish reason is "stop", improving user experience during chat interactions. These changes improve the reliability of message handling and reduce unnecessary UI flashes.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/web/components/chat/context.tsx">
<violation number="1" location="apps/mesh/src/web/components/chat/context.tsx:673">
P2: This new `finishReason === "stop"` guard suppresses notifications for normal finishes. Per prior guidance, notifications should only be suppressed when `isAbort`, `isDisconnect`, or `isError` is true; those cases already return earlier. Remove the finishReason gating so notifications fire for all successful finishes.
(Based on your team's feedback about not suppressing notifications based on finishReason.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Modified the notification handling in ChatProvider to trigger notifications whenever enabled, regardless of finish reason. This change simplifies the notification logic and ensures users are consistently informed during chat interactions. This update enhances the user experience by providing timely notifications.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this contribution about?
Screenshots/Demonstration
How to Test
Migration Notes
Review Checklist
Summary by cubic
Adds distributed Decopilot run control with cross-pod cancel and late-join stream replay over NATS/JetStream via a shared NATS connection used by Event Bus and Decopilot. Runs continue in the background after disconnect; the UI and SDK show live status, step counts, and cancel controls with local fallbacks when NATS is unavailable.
New Features
Migration
Written for commit 342f791. Summary will update on new commits.